-
Notifications
You must be signed in to change notification settings - Fork 163
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
uchardet.dll が存在したらそれを使って文字エンコーディングの検出が行われるように処理追加 #1726
Conversation
❌ Build sakura 1.0.3908 failed (commit 0ef1762133 by @beru) |
✅ Build sakura 1.0.3909 completed (commit 0ff761fba4 by @beru) |
✅ Build sakura 1.0.3911 completed (commit 5a17b768f1 by @beru) |
@@ -30,39 +30,21 @@ CharsetDetector::CharsetDetector() noexcept | |||
, _csd(nullptr) | |||
{ | |||
_icuin.InitDll(); | |||
_uchardet.InitDll(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icu4cのためのコードを変更することに違和感があります。
sakura_core/charset/icu4c/CharsetDetector.cpp
👇コピーして新規作成、icu4cの要素をuchardetの要素で置き換える
sakura_core/charset/uchardet/CharsetDetector.cpp
というアプローチは難しいのでしょうか?
icu4cは確か、バージョンごとに関数名が異なる「変な仕様」で、ビルドするのが大変な構造になっていたような記憶があります。uchardetのバージョン構造がどうなっているか分からないですが、icu4cの仕様に引きずられてビルドが難しくなるのはマイナスだと思います。icu4cの実装をコピペしてuchardetに必要なモノだけを定義していったほうがすっきりした実装になると気がするので、そういう方式がおすすめです。(中身の詳細は、きっと誰も見ないですしw
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
コピペして分けるのも有りかと思います。しかし namespace 使わない場合、重複するクラス名を付けられないので、既存の CharsetDetector
じゃない名前のクラスにする必要が出てきそうです。uchardetCharsetDetector
とか?
どうして既存の CharsetDetector
クラスの実装を再利用したかというと、その方がコードの変更量が少なくて済むからです。CharsetDetector
クラスは CCodeMediator::CheckKanjiCode
メソッドで使われていますが、もし別のクラスを追加する場合はここの記述も変えないといけないので。
あと CharsetDetector
クラスの public な interface は ICU4C の interface に依存していないので、uchardet を使った文字エンコーディング判定処理もここでやってしまうのが適切だと判断しました。
まぁそれならそれで、sakura_core\charset\icu4c
フォルダではなくて上のフォルダにファイルを移動する変更も合わせて行った方が良いかもしれないですね。あと Pimplイディオム を使って CharsetDetector.h
ファイルからは ICU4C や uchardet のDLLのラッパーの存在を表面化させないようにした方が良い気がします。
ICU4C と uchardet が返す文字セット名が同じかどうかをまず調べる必要がありそうです(uchardet は iconv と同じ名前を返すらしいです、ICU4Cは良くわかりません)。あと今は連続する if 文で文字セット名⇒サクラエディタ内部コードの変換を行っていますが、ICU4C や uchardet は各国の様々な文字エンコーディングに対応しているのできちんとそれらをWindowsのコードページIDに変換するように記述を追加したいです。
あと if 文の文字列判定を繰り返すのは処理効率が良くないので std::unordered_map
で表引きする実装にしたいです。まぁ数が少ないうちは(数十ぐらいだったら)そんなに気にしなくても良いかもしれませんが。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
CharsetDetector のファイルは親フォルダに移動しました。
ICU4Cを使う場合の処理は従来通りに戻しました。
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
icu4cは確か、バージョンごとに関数名が異なる「変な仕様」で、ビルドするのが大変な構造になっていたような記憶があります。uchardetのバージョン構造がどうなっているか分からないですが、icu4cの仕様に引きずられてビルドが難しくなるのはマイナスだと思います。icu4cの実装をコピペしてuchardetに必要なモノだけを定義していったほうがすっきりした実装になると気がするので、そういう方式がおすすめです。(中身の詳細は、きっと誰も見ないですしw
それについては sakura_core\extmodule\CIcu4cI18n.cpp
でカバーされているので、uchardet を使う場合には影響はないです。uchardet の DLL を扱う対応は別のファイル sakura_core\extmodule\CUchardet.cpp
で行われています。
CharsetDetector
クラスは元々ICU4Cしか使っていなかったのですが今回uchardetも使うように処理を追加しました。このクラスに変更を入れずに別のクラスを使うようにした方が良い場合はそうします。
uchardetライブラリを使う場合は検出したエンコーディング名からWindows code pageへの変換は std::unordered_map による表引きを行うように変更
SonarCloud Quality Gate failed. |
✅ Build sakura 1.0.3920 completed (commit 03f33d6d7a by @beru) |
uchardetの紹介ページに、 c++のオリジナルがあるならそちらを使ったほうがプログラムしやすいんではないかと。 あと、今更気付きましたが、uchardet.dllの入手方法が書いてないっす。 たぶんソースコードを落としてcmakeでビルドしたんだと思うんですが、これで「手順通りテスト」するのはかなり高度な気がしました。 とはいえ、これが入れば #1725 は解決するので、入れてしまった方がいいような気もします。。。 |
そのような説明はされていないと思います。おそらく下記の英文を適当に読んだのではないかと思いますが、
uchardetはMozillaの全世界の文字セット検出ライブラリのC++実装へのC言語バインディングから始まりました。 というような意味合いの文だと思います。
自分もVC++だとビルド出来ないかもと思ったんですが試してみたら問題が無かったです。
こうすると
uchardet が対応しているエンコーディングに GB2312 はないですがそのスーパーセットの GB18030 はあるので、それでなんとかなればいいなぁと思います。 |
C++ライブラリのC言語バインディングってなんですか? バインディングは移植じゃねぇよ、もそれはそれで正しいと思います。 dllは、zipを貼りますか・・・。
そればっかりは中国の人に聞いてみないと分からないですね。 |
#1104 ではDLLファイルは提供していないのだし、これについてもやらなくてもいいんじゃないかと思ってます。「それとこれとは違う!」と言われたら「あ、はい」としか返せませんが…。
そうですね。それにuchardetは世界各国で使われている色々なレガシーなエンコーディングに対応しているので、本当にきちんと動作するかは個人ではなかなか調べきれないでしょうね。 他に気になる点が色々ありますが、このPRで対応するべきか微妙です。
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
動いてそうなのでいいんじゃないかと思います。
uchardetのwindows向けバイナリは、配布されていないように見えました。
動作確認のためにローカルビルドしたdllは以下になります。
uchardet_x86.zip
uchardet_x64.zip
文字コードセットについて、公的機関(≒IANA)が管理しているのは「名前」だったような気がします。文字コードセットの指定を「番号」ではなくて「名前」で指定できるようにしたいなぁ、と考えているのはちょっと別な話になりますけれども。
レビューありがとうございます。Mergeします。 |
PR の目的
uchardet という文字エンコーディングの検出を行うライブラリに対応するのが目的です。
カテゴリ
PR の背景
この uchardet というライブラリは Mozilla で使われていたライブラリのようです。今のFirefoxはrustで書かれた chardetng を使っているようです。
VS Codeは jschardet を使っているみたいです。jschardet は Python製の chardet を元にしていて、それの更に元になっているのがC++で書かれた uchardet のようです。
PR のメリット
#1104 で対応がされたICU4Cと比べてuchardetの方がビルドが容易でバイナリサイズも小さいです。
PR のデメリット (トレードオフとかあれば)
特にないと思いますが、あえて言うとDLLの確認が増える分だけ微妙に遅くなるかもしれません。
仕様・動作説明
sakura_core/charset/icu4c/CharsetDetector.h
とsakura_core/charset/icu4c/CharsetDetector.cpp
の実装を利用しています。フォルダ名が icu4c なのであまり良くないかもしれません。
ICU4C の
ucsdet_getName
が返す文字列と uchardet のuchardet_get_charset
が返す文字列に互換性があるかはきちんと確認していません。大丈夫だといいな。PR の影響範囲
文字コード検出処理に関係しますが、デフォルトでは
uchardet.dll
ファイルがパスが通った場所に無ければ処理が有効にならないので、あまり影響は出ないと思います。テスト内容
テスト1
手順
x64\Debug
フォルダにx64 Debugビルドしたuchardet.dll
を配置テスト2
手順
x64\Release
フォルダにx64 Releaseビルドしたuchardet.dll
を配置関連 issue, PR
#1104 #1725
参考資料
https://docs.microsoft.com/en-us/windows/win32/intl/code-page-identifiers
https://www.freedesktop.org/wiki/Software/uchardet/
https://xz5012.wordpress.com/2017/11/10/my-chinese-version-of-the-moon-over-the-mountain/